-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use alignWith in alignMergeWith #4676
base: main
Are you sure you want to change the base?
Conversation
`alignWith` is usually overridden for efficiency
Thank you! cats/laws/src/main/scala/cats/laws/AlignLaws.scala Lines 45 to 46 in 7a30dc2
|
@satorg that law already exists. Am I missing something? |
I mean, a law that would check a correspondence between |
Done, but it seems like now unrelated tests are failing |
import cats.syntax.align.* | ||
import cats.syntax.functor.* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a nit but maybe these changes to the import style are unnecessary. Although it's Scalafmt duty, we use scala213source3
dialect that allows cross-version syntax constructions to be put together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess Scalafmt is not configured to update the imports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, we have this
runner.dialectOverride.allowStarWildcardImport = false
and it appears to be not working (or maybe it works oppositely to my expectations)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hah, I thought we'd like to keep the Scala 2 style :) But okay I don't strongly disagree with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... I doesn't exactly compile for all the modules in the repo. I guess, it could be a reason why it was disabled 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted. Just keep in mind that if you let Intellij optimize imports it will replace them with star
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC, the import style was configurable there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps, I didn't check but it takes if from sbt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
alignWith
is usually overridden for efficiency